Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add virtual properties for values and indices #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Dec 25, 2022

This is a full implementation of #316. Virtual properties for values and indices are added and documented.

This differs from the proposed implementation in #316 in that the properties return IdOffsetArray rather than UnitRanges.

Another discovered benefit is the ability to forward the properties to construct another IdOffsetRange.

        _values = 5:8
        _indices = 6:9
        id = IdOffsetRange(values=_values, indices=_indices)
        @test id.values === values(id)
        @test id.values == _values
        @test id.indices === eachindex(id)
        @test id.indices == _indices
        id2 = IdOffsetRange(; id.values, id.indices)
        @test id == id2
        id3 = IdOffsetRange(; id.indices, id.values)
        @test id == id3

@codecov
Copy link

codecov bot commented Dec 25, 2022

Codecov Report

Merging #318 (2945d61) into master (08dc371) will decrease coverage by 1.29%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   96.45%   95.16%   -1.30%     
==========================================
  Files           5        5              
  Lines         451      434      -17     
==========================================
- Hits          435      413      -22     
- Misses         16       21       +5     
Impacted Files Coverage Δ
src/axes.jl 97.61% <50.00%> (-2.39%) ⬇️
src/utils.jl 96.00% <0.00%> (-2.00%) ⬇️
src/OffsetArrays.jl 97.50% <0.00%> (-0.80%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jishnub
Copy link
Member

jishnub commented Jan 2, 2023

Looking at the example from #316,

julia> Base.@kwdef struct MyIdOffsetRange
                  values::UnitRange
                  indices::UnitRange
              end
MyIdOffsetRange

julia> x = MyIdOffsetRange(values=5:8, indices=5:8)
MyIdOffsetRange(5:8, 5:8)

julia> x.values
5:8

julia> x.values === x
false

In this example, the returned results correspond exactly to the displayed ones, as both directly read the fields of the struct. However, for an IdOffsetRange, we obtain another IdOffsetRange, which isn't what was displayed.

julia> id
OffsetArrays.IdOffsetRange(values=5:8, indices=6:9)

julia> id.values
OffsetArrays.IdOffsetRange(values=5:8, indices=6:9)

julia> id.values === id
true

This isn't an objection to the PR, but I wonder if we need to be consistent here?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 3, 2023

The consistency choice I made here was for

id.values === values(id)

values is part of the API already.

If you really wanted a UnitRange, this could work:

UnitRange(id.values)

For that reason, I added it to the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants